Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(explore): add config for default time filter #21879

Merged
merged 2 commits into from
Oct 20, 2022
Merged

feat(explore): add config for default time filter #21879

merged 2 commits into from
Oct 20, 2022

Conversation

Usiel
Copy link
Contributor

@Usiel Usiel commented Oct 20, 2022 •
edited
Loading

SUMMARY

First change to complete #13125: We add a new configuration DEFAULT_TIME_FILTER to allow admins to set a global default time filter for any new explore charts. By default this is set to None, which replicates the previous behavior ("No filter"). To get a default filter of -1w the configuration can be set to 'Last week' or any other value understood by Superset.

There is a minor change in behavior UPDATE_FORM_DATA_BY_DATASOURCE (when switching dataset in explore), the time filter is not reset anymore. This is similar to the behavior of other globally configured filters such as the row limit for explore, which also do not reset.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

New explore chart with DEFAULT_TIME_FILTER = None:

New explore chart with DEFAULT_TIME_FILTER = "Last week":

TESTING INSTRUCTIONS

Test default behavior

  1. Go to datasets and click on users (example dataset)
  2. Add updated (time) as a dimension (helps to verify)

Expected: No time filter is applied when running the chart

Test with new configuration

  1. Set DEFAULT_TIME_FILTER = "2020-06-01T00:00:00 : now" in config.py
  2. Go to datasets and click on users (example dataset)
  3. Add updated (time) as dimension (helps to verify)

Expected: Default time filter is applied

ADDITIONAL INFORMATION

@Usiel Usiel force-pushed the feat/global-default-time-range branch 3 times, most recently from 99ea6dd to d2b48ee Compare October 20, 2022 04:10
@codecov
Copy link

codecov bot commented Oct 20, 2022 •
edited
Loading

Codecov Report

Merging #21879 (0cdfadc) into master (28c7636) will decrease coverage by 0.16%.
The diff coverage is 75.00%.

❗ Current head 0cdfadc differs from pull request most recent head 809b365. Consider uploading reports for the commit 809b365 to get more accurate results

@@            Coverage Diff             @@
##           master   #21879      +/-   ##
==========================================
- Coverage   66.90%   66.73%   -0.17%     
==========================================
  Files        1806     1806              
  Lines       69130    69131       +1     
  Branches     7391     7391              
==========================================
- Hits        46249    46136     -113     
- Misses      20970    21084     +114     
  Partials     1911     1911
Flag Coverage Δ
hive ?
mysql 78.35% <100.00%> (+<0.01%) ⬆️
postgres 78.41% <100.00%> (+<0.01%) ⬆️
presto ?
python 81.13% <100.00%> (-0.35%) ⬇️
sqlite 76.90% <100.00%> (+<0.01%) ⬆️
unit 51.06% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...d/packages/superset-ui-core/src/query/constants.ts 100.00% <ø> (ø)
...nts/controls/DateFilterControl/DateFilterLabel.tsx 35.71% <ø> (ø)
superset-frontend/src/explore/fixtures.tsx 75.00% <ø> (ø)
...et-frontend/src/explore/reducers/exploreReducer.js 38.09% <ø> (ø)
superset/views/base.py 75.70% <ø> (ø)
...set-frontend/src/explore/actions/hydrateExplore.ts 42.10% <50.00%> (ø)
superset/config.py 91.66% <100.00%> (+0.02%) ⬆️
superset/db_engines/hive.py 0.00% <0.00%> (-85.19%) ⬇️
superset/db_engine_specs/hive.py 71.14% <0.00%> (-16.21%) ⬇️
superset/db_engine_specs/presto.py 81.05% <0.00%> (-6.73%) ⬇️
... and 10 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Usiel
feat: add config for default explore time filter
b9bc98d 
Related to issue apache#13125

First change to complete apache#13125: We add a new configuration `DEFAULT_TIME_FILTER` to allow admins to set a global deafult time filter for any explore charts. By default this is set to `None`, which replicates the previous behavior. To get a default filter of -1w the configuration can be set to `'Last week'`.

There is a minor change in behavior `UPDATE_FORM_DATA_BY_DATASOURCE`, the time filter is not reset anymore (the time dimension does still). This is similar to the behavior of other globally configured filters such as the row limit for explore.
@Usiel Usiel force-pushed the feat/global-default-time-range branch from f1a73af to b9bc98d Compare October 20, 2022 05:23
zhaoyongjie
zhaoyongjie reviewed Oct 20, 2022
View reviewed changes
superset/config.py Outdated
@@ -153,6 +153,9 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]:
SAMPLES_ROW_LIMIT = 1000
# max rows retrieved by filter select auto complete
FILTER_SELECT_ROW_LIMIT = 10000
# default time filter in explore
# values may be "Last day", "Last week", "<ISO date> : now", etc.
DEFAULT_TIME_FILTER = None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one nite, we should keep this config as before, the None and the No filter will generate different time expressions. the None will generate : today(col <= today()), and No filter will generate :(empty filter).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed 👍

@Usiel Usiel force-pushed the feat/global-default-time-range branch from 77a6b26 to 30e0f1a Compare October 20, 2022 08:22
@Usiel
Set explicit default for DEFAULT_TIME_FILTER
809b365 
Using "No filter" string instead of None to avoid issues downstream if None is not properly handled.
@Usiel Usiel force-pushed the feat/global-default-time-range branch from 30e0f1a to 809b365 Compare October 20, 2022 08:29
zhaoyongjie
zhaoyongjie approved these changes Oct 20, 2022
View reviewed changes
Copy link
Member

@zhaoyongjie zhaoyongjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This config item is very useful in the production environment. LGTM, Thanks for the contribution!

@zhaoyongjie zhaoyongjie merged commit 9a063ab into apache:master Oct 20, 2022
@Usiel Usiel mentioned this pull request Oct 21, 2022
Closed
@Utsavjain4561
Copy link

Hi, we are running superset 1.5.1rc1 right now and i guess in that there is no such support of DEFAULT_TIME_FILTER but i can see there a prop of DEFAULT_TIME_RANGE so is it possible if we hardcode its value to Last week and will it work for existing and new charts to have a default time range of Last week ?

@zhaoyongjie
Copy link
Member

Hi, we are running superset 1.5.1rc1 right now and i guess in that there is no such support of DEFAULT_TIME_FILTER but i can see there a prop of DEFAULT_TIME_RANGE so is it possible if we hardcode its value to Last week and will it work for existing and new charts to have a default time range of Last week ?

Yes, it should be same as DEFAULT_TIME_FILTER in config.py.

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhaoyongjie zhaoyongjie zhaoyongjie approved these changes

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 2.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Usiel @Utsavjain4561 @zhaoyongjie @mistercrunch